Skip to content

London | 26-ITP-Sep | Diana Ausiejute | Sprint 2 |Implement and Rewrite Tests - #1672

Open
ausiejute wants to merge 12 commits into
CodeYourFuture:mainfrom
ausiejute:coursework/sprint-3-implement-and-rewrite
Open

ausiejute wants to merge 12 commits into
CodeYourFuture:mainfrom
ausiejute:coursework/sprint-3-implement-and-rewrite

Conversation

@ausiejute

Copy link
Copy Markdown

Learners, PR Template

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

Task code

CYF-1059

Changelist

Wrote function implementations and tests in Node and Jest covering a variety of cases.

@ausiejute ausiejute added 📅 Sprint 2 Assigned during Sprint 2 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 10, 2026
@dkrest1 dkrest1 self-assigned this Oct 11, 2026
@dkrest1
dkrest1 self-requested a review October 11, 2026 17:37
@dkrest1 dkrest1 added the Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. label Oct 11, 2026
// execute the code to ensure all tests pass.

export function getAngleType(angle) {
// TODO: Implement this function

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think happens when a function is declared twice, once at the top and again right inside itself without the inner one ever being called?

@ausiejute ausiejute Oct 12, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to have a read about it and it seems that the function would be redefined inside of itself resulting in the inner function being called instead, but only if it needed to be a recursive one. In this situation, it would look confusing, but would still do the intended job.
However, I think there might have been a misunderstanding with the diff. I only changed the function declaration/export style slightly, so the original declaration in red was replaced by the implementation in green; the function isn't declared twice. I wondered about this because a similar comment was added to another task (implement-isProperFraction) on a function that was also in the deleted code. Could you clarify if I'm missing something?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are spot on! That was entirely on my end due to viewing the diff in Unified View, which made the replaced code look like a duplicate declaration. Your implementation looks completely correct, no double declaration at all. Thanks for clearing that up, and great eye on catching that diff quirk!

// execute the code to ensure all tests pass.

export function getAngleType(angle) {
// TODO: Implement this function

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if someone passes a non-number like "90" or null here? Should we add validation

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I did not take that into consideration. Passing a regular string would return an "invalid angle", but an empty one or null would not

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great! changes implemented


// TODO: Write tests to cover all cases, including boundary and invalid cases.
// Example: Identify Right Angles

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add tests for zero and negative numbers too, just to check how those boundary cases are handled

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome update! Checking for zero and negative numbers makes the test suite much stronger. Everything looks solid here.

// execute the code to ensure all tests pass.

export function isProperFraction(numerator, denominator) {
// TODO: Implement this function

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this function is declared twice here. What do you think happens to that inner function since it's never called

@ausiejute ausiejute Oct 12, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the function declaration/export style slightly and if you view the file it's more clear that the function has been declared once

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah!, great catch


// TODO: Write tests to cover all cases.
// What combinations of numerators and denominators should you test?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the denominator is 0? Should we add a test case to see how the function handles that?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, I see you’ve already implemented this fix! Everything looks good here.

return "invalid";
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any debug logging at the bottom of the file that we should remove before finalizing?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I couldn't find any

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check other comments about this

// execute the code to ensure all tests pass.

export function getCardValue(card) {
// TODO: Implement this function

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just like the other files, this function is declared twice. What happens to the inner function since it isn't exported or called

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once again, the function is declared once.. The export statement simply makes that existing function available to other modules; it does not declare another function or create an inner function. Your comments are written on the deleted sections

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out! Just like in the other exercise, this was down to how I was viewing the diff in GitHub (Unified View showing the old red lines alongside the new green lines). Your code actually only declares the function once and is completely fine. Great job on the implementation!

cardRank == "10"
) {
return Number(cardRank);
} else {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The instructions above mention that an invalid card string should throw an error. How does returning the string "invalid" compare to throwing an error

@ausiejute ausiejute Oct 12, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returning the string "invalid" would still allow the program to continue running without interruption, whereas throwing an error indicates a more serious issue that requires immediate attention and handling

Edit: I see now that some misunderstandings happened due to us looking at different versions of the file

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spot on! You've captured the exact difference in control flow—returning a value allows execution to continue silently, whereas throwing an error halts execution and forces the caller to handle the edge case explicitly.

Also, completely agree regarding the diff mix-up thanks for bearing with me while I sorted out my view settings!

) {
return Number(cardRank);
} else {
return "invalid";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens to the throw new Error statement right after return "invalid"; on line 57? Will that error ever actually be reached by the program?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 56-59: assert.throws(
() => getCardValue(""),
Error,
"Should throw an error for empty input"

throw new Error(`Expected a number followed by a suit, but got "${card}"`);
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any debug logging (console.log) at the bottom of the file that we should remove before finalizing or committing our code?

@ausiejute ausiejute Oct 12, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function getCardValue(card) {
  // implementation
}

export { getCardValue };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all sorted now, spot on!

// To learn how to test whether a function throws an error as expected in Jest,
// please refer to the Jest documentation:
// https://jestjs.io/docs/expect#tothrowerror
// -----number cards------

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GREAT WORK HERE!
but notice how all the number card tests (2 through 9) follow the exact same pattern. Is there a built-in Jest feature (like test.each) we could use here to keep our tests DRY (Do not repeat yourself) and avoid repeating code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! it's great to know how to avoid repeating the code

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice Refactor with test.each

@dkrest1 dkrest1 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job working through these! Your logic and test coverage for edge cases and errors are really strong.

Before finalizing, just double-check for:

  1. Duplicate function declarations in your implementation files.

  2. Leftover debug logs (console.log) that can be removed.

DRY tests (like using test.each for repetitive test cases).

Thanks for sharing your progress, keep up the great work!

@dkrest1 dkrest1 added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Oct 11, 2026
@ausiejute
ausiejute requested a review from dkrest1 October 12, 2026 23:45
@ausiejute ausiejute added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 12, 2026
@dkrest1 dkrest1 added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Reviewed Volunteer to add when completing a review with trainee action still to take. labels Oct 14, 2026

@dkrest1 dkrest1 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this PR, Diana! 🚀

Thank you for working through all the feedback so thoroughly and for bearing with me through the initial diff viewer confusion. You’ve done a great job restructuring the tests, applying edge-case coverage for negative numbers and zero, and understanding the core mechanics between returning error strings vs. throwing errors.

Everything looks clean, accurate, and well-tested now. Approved!

@dkrest1 dkrest1 added Reviewed Volunteer to add when completing a review with trainee action still to take. Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. Reviewed Volunteer to add when completing a review with trainee action still to take. labels Oct 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed. 📅 Sprint 2 Assigned during Sprint 2 of this module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants